wip: cleaning up the specs list and swapping it to be a filetree #15483

JessicaSachs · 2021-03-15T11:01:10Z

This PR rewrites the DOM structure of the SpecList, as well as attempts to decouple the SpecList model from rendering the file tree.

I will need to pair with someone on this PR because it still needs a lot of work to remove technical debt before it is good to merge.

Closes

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

Have tests been added/updated?
Has the original issue or this PR been tagged with a release in ZenHub?
Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?
Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2021-03-15T11:01:13Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2021-03-15T11:13:38Z

Test summary

9384 • 0 • 119 • 3 • 1

Run details


Project	cypress
Status	Passed
Commit	`f1a4901`
Started	Mar 15, 2021 11:01 AM
Ended	Mar 15, 2021 11:13 AM
Duration	12:22 💡
OS	Linux Debian - 10.5
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	cypress/integration/retries.ui.spec.js		1
1	runner/cypress retries.ui.spec > opens attempt on each attempt failure for the screenshot, and closes after test passes

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

dmtrKovalenko · 2021-03-15T12:47:48Z

npm/design-system/src/components/FileExplorer/FileExplorer.module.scss

+  white-space: nowrap;
+}
+
+.ul {


Just curios why classes are named like html tags, looks a little confusing as for me.

dmtrKovalenko · 2021-03-15T12:48:08Z

npm/design-system/src/components/FileExplorer/FileExplorer.module.scss

@@ -0,0 +1,73 @@
+@use '../../index.scss' as *;


Suggested change

@use '../../index.scss' as *;

@import '../../index.scss';

No. You want @use. It deduplicates imports and has many other benefits. https://css-tricks.com/introducing-sass-modules/#import-files-with-use

Oh, how much pain in the ass just to avoid css-in-js :D
I will never understand this probably

Why did we decide not to use css-in-js?

dmtrKovalenko · 2021-03-15T12:48:36Z

npm/design-system/src/components/FileExplorer/FileExplorer.spec.tsx

+]
+
+describe('FileExplorer', () => {
+  it('renders', () => {


Any assertions? At least cy.persySnapshot?

No, I was using it as a playground, not for specs. It still needs specs... just was trying to get something up for the src to be reviewable.

dmtrKovalenko · 2021-03-15T12:49:06Z

npm/design-system/src/components/FileExplorer/FileExplorer.tsx

+            updateItem(item, idx, { isOpen: !item.isOpen })
+          }
+
+          return (<React.Fragment key={item.shortName}>


Suggested change

return (<React.Fragment key={item.shortName}>

return (

<React.Fragment key={item.shortName}>

dmtrKovalenko · 2021-03-15T12:49:46Z

npm/design-system/src/components/FileExplorer/FileExplorerItem.tsx

+import styles from './FileExplorer.module.scss'
+import cs from 'classnames'
+
+const icons: Record<string, any> = {


Suggested change

const icons: Record<string, any> = {

const icons: Record<string, { icon: typeof reactTs }> = {

Or pass the right type

Thanks. There's a lot I don't really understand how to do.

dmtrKovalenko · 2021-03-15T12:50:26Z

npm/design-system/src/components/FileExplorer/FileExplorerItem.tsx

+  folderClosed: { icon: folderClosed },
+}
+
+const getExt = (path: string) => {


Why not to use https://nodejs.org/api/path.html#path_path_extname_path?

Ohh that is frontend. Sorry :D

dmtrKovalenko · 2021-03-15T12:50:47Z

npm/design-system/src/components/FileExplorer/FileExplorerItem.tsx

+  }
+
+  return (
+    <a


Please use <button for a11y reasons

Why? Isn't a more correct because we're doing a hash navigation change?

You are passing both onClick and href which is really weird. I don't understand this – it looks like you are trying to sync url and state which is a problem all the time. We need to have a single source of trust.

So if you want to use url – you need to subscribe to url changes and use hash without any internal state

href is fine as long as onClick is calling preventDefault. This is the "correct" way to build buttons that perform as links, so that you still get standard browser right/middle click behavior.

Yes, but it is unclear that it is doing prevent default. Maybe not, how would you know from this code? If it is – we need some kind of <Link /> component. But we don't really have any kind of routing, so it looks confusing

dmtrKovalenko · 2021-03-15T12:51:19Z

npm/design-system/src/components/FileExplorer/helpers/makeFileHierarchy.ts

+ * Split specs into group by their
+ * first level of folder hierarchy
+ *
+ * @param {Array<{name: string}>} specs


It looks weird, because we have different type in the function declaration

Suggested change

* @param {Array<{name: string}>} specs

Yeah this was copy/paste foul.

dmtrKovalenko · 2021-03-15T12:51:36Z

npm/design-system/src/components/FileExplorer/types.ts

+export type FolderOrFile = File | Folder
+
+export interface File extends FileLike {
+	type: 'file'


Why indention here?

dmtrKovalenko · 2021-03-15T12:51:50Z

packages/runner-ct/src/SpecList/SpecList.tsx


+          return props.selectedSpecs.includes(item.absolute)
+        }
+        }/>


Suggested change

}/>

}

/>

dmtrKovalenko · 2021-03-15T12:54:42Z

Also, it is weird that components like FileTree are inside the design system. Cause it looks like more "end" component, maybe better place will be inside runner-ct?

lmiller1990

Cool! Some tentative comments, we can pair or I can make a PR to make some improvements. Primary smell is the useState, we should not be maintaining the files list in two places!

lmiller1990 · 2021-03-15T15:06:25Z

npm/design-system/src/components/FileExplorer/FileExplorer.spec.tsx

+    isOpen: false,
+  },
+  // @ts-ignore
+  { name: 'bar/foo.spec.tsx' },


why do we need ts-ignore?

This is wrong. It's because I was hacking on this file and hadn't finished the types.

lmiller1990 · 2021-03-15T15:17:35Z

npm/design-system/src/components/FileExplorer/FileExplorer.spec.tsx

+  it('renders', () => {
+    mount(<>
+      <FileExplorer files={files} />
+    </>)


Don't need fragment here

lmiller1990 · 2021-03-15T15:18:12Z

npm/design-system/src/components/FileExplorer/FileExplorer.tsx

+  return extensionMatches ? extensionMatches[1] : ''
+}
+
+export const FileExplorer: React.FC<FileExplorerProps> = (props) => {


spacing is quite confusing here, can we make it a bit cleaner

lmiller1990 · 2021-03-15T15:18:58Z

npm/design-system/src/components/FileExplorer/FileExplorer.tsx

+
+export const FileExplorer: React.FC<FileExplorerProps> = (props) => {
+  return (<nav className={cs(props.className || '', styles.nav)}><FileTree files={makeFileHierarchy(props.files) || []} isSelected={props.isSelected || (() => {})} onClick={props.onClick || (() => {})}></FileTree></nav>)
+}


Do we need all the || () => {}? Can we just make the props nullable?

And even if not, you should use ??

I don't think we can use ?? with our version of ts? I'm not sure.

We are not using ts for compilation it is extremely bad for this (if we are we should stop). Babel will handle this much better. ?? is a stage 3 so preset-env will be enough and I think it will work out of the box

?? is a stage 3 so preset-env will be enough and I think it will work out of the box

https://babeljs.io/docs/en/babel-plugin-proposal-optional-chaining if not

However, @dmtrKovalenko, if the TS version we're using doesn't support it, we'll get errors from the LSP

lmiller1990 · 2021-03-15T15:21:09Z

npm/design-system/src/components/FileExplorer/FileExplorer.tsx

+
+export const FileTree: React.FC<FileTreeProps> = (props) => {
+  const depth = props.depth || 0
+  const [files, setFiles] = React.useState(props.files || [])


This seems incorrect - now we have two sources of truth. Why do we need two sources of truth? Can't we just use props.files? Design system should not have internal state, ideally everything should be a function of props

Should I be passing a callback all the way down and making the top-level component mutate files?

Also, it is weird that components like FileTree are inside the design system. Cause it looks like more "end" component, maybe better place will be inside runner-ct?

I think it makes sense because we will use it in runner-ct as well as with the node UI (possibly desktop-gui?). There's also some other benefits.

Technically we should have separate design system and Cypress component library folders (where the only components the design system has are individual atoms), but I don't know that it's worth going to that extreme now.

Yes. You're correct. I'd be happy just to get our design tokens plumbed first and also keep reusable UI patterns separate from domain models.

lmiller1990 · 2021-03-15T15:22:14Z

npm/design-system/src/components/FileExplorer/FileExplorerItem.tsx

+  style?: React.CSSProperties
+  getHref? (item: FolderOrFile): string
+  depth: number
+  onFocus: any


Can we type these?

lmiller1990 · 2021-03-15T15:23:42Z

npm/design-system/src/index.ts

@@ -2,6 +2,8 @@ export * from './components/Button'

 export * from './components/CypressLogo/CypressLogo'

+export * from './components/FileExplorer/FileExplorer'


I think it's idiomatic to add a index.ts that exports everything. export * from './FileExplorer. Then this becomes export * from './components/FileExplorer'

This was the pattern that @agg23 followed, so I did the same. I don't care one way or another. I've also seen the index.ts pattern and prefer it for convenience, but find it difficult when trying to search for things.

I don't care right now. I imagine I will be changing around exports significantly when there starts to be atoms vs final components, so whatever is fine by me.

JessicaSachs · 2021-03-15T20:14:55Z

Also, it is weird that components like FileTree are inside the design system. Cause it looks like more "end" component, maybe better place will be inside runner-ct?

I think it makes sense because we will use it in runner-ct as well as with the node UI (possibly desktop-gui?). There's also some other benefits.

Having it decoupled from the model makes it easier to test and more portable because it's not reliant on the global stylesheets of runner-ct.

Additionally, if we know we'll reuse the pattern, it'll be better to decouple from domain model + global styles early on before runner-ct's style leaks into the component. Once the component's styles are polluted, it's very hard and risky to split them out.

agg23 · 2021-03-15T20:18:10Z

Also, it is weird that components like FileTree are inside the design system. Cause it looks like more "end" component, maybe better place will be inside runner-ct?

I think it makes sense because we will use it in runner-ct as well as with the node UI (possibly desktop-gui?). There's also some other benefits.

Technically we should have separate design system and Cypress component library folders (where the only components the design system has are individual atoms), but I don't know that it's worth going to that extreme now.

dmtrKovalenko · 2021-03-15T20:26:23Z

This file explorer should not be a part of any shared library because it is too coupled with a context of runner-ct. Making it decoupled will be a pain I think. So I don't see any problems in putting it right in the runner-ct. Am I missing something?

dmtrKovalenko · 2021-03-15T20:33:15Z

npm/design-system/src/components/FileExplorer/FileExplorerItem.tsx

+      onClick={(e) => {
+        props.onClick(e, props.item)
+        onClick(e, props.item)
+      }}


We can move it to function for readability

function handleClick(e: React.MouseEvent<HtmlLinkElement>) { props.onClick(e, props.item); if (props.item.onClick) { props.item.onClick(e, props.item) } } onClick={handleClick}

dmtrKovalenko · 2021-03-15T20:34:00Z

npm/design-system/src/components/FileExplorer/FileExplorerItem.tsx

+      { props.item.isSelected }
+      <li
+        style={{ ...props.style, ...inlineStyles.li }}
+        className={styles.li}>


Suggested change

className={styles.li}>

className={styles.li}

>

JessicaSachs · 2021-03-15T22:11:29Z

This file explorer should not be a part of any shared library because it is too coupled with a context of runner-ct. Making it decoupled will be a pain I think. So I don't see any problems in putting it right in the runner-ct. Am I missing something?

I mean... it works and it's easy to test, and will pay us forward if we use it for non-specs (workbench mode? stories? idk). I don't see a reason to revert it.

In general, I don't really see a reason not to make most things decoupled from domain objects... if it's presentational, make it presentational 🤷🏻 Let's DM sometime, I'm curious as to why it's a good idea to couple.

lmiller1990 · 2021-03-16T00:54:59Z

I am on board with @dmtrKovalenko regarding where this belongs. At least to me, a design system should be more generic. Things like buttons, lists, modals, etc. A spec list doesn't really fit into this in my opinion. What would belong here is a <TreeView>. This Spec List is coupled to specs (spec-specific icons, checking for extensions, etc).

I think the way to make it decoupled would be something like

<TreeView
  leafComponent={Folder}
  nodeComponent={File}
  data={specHierarchy}
/>

(Trivial example). It's similar to using <template> and <slot> in Vue.

Then you could re-use it for various other tree views (perhaps someone else needs a tree view, but does not want to have icons, etc). In Vue this is called a "renderless component". React often refers to this pattern is "render as a function", I think. So what we would need is

Generic <TreeView> component (no render, just the model)
Spec List components. <File>, <Folder> etc. This is where things like icons and extension logic might live.
Compose them together to create a <SpecList>.

Anyway, I think we can discuss this further, for now I will push some simple changes to get this over the line and fix the tests.

Regarding CSS modules vs CSS-in-JS, I don't have much experience around either, it's super annoying how you cannot see the class names in the DOM since CSS modules make them into a hash. I wonder if there is some flag to show something a bit more readable in the DOM.

JessicaSachs · 2021-03-16T18:22:19Z

Closing in favor of #15513

wip: cleaning up the specs list and swapping it to be a filetree

f1a4901

dmtrKovalenko reviewed Mar 15, 2021

View reviewed changes

lmiller1990 suggested changes Mar 15, 2021

View reviewed changes

dmtrKovalenko reviewed Mar 15, 2021

View reviewed changes

lmiller1990 mentioned this pull request Mar 16, 2021

feat(design-system): FileExplorer #15513

Merged

JessicaSachs closed this Mar 16, 2021

	return (<React.Fragment key={item.shortName}>
	return (
	<React.Fragment key={item.shortName}>

	const icons: Record<string, any> = {
	const icons: Record<string, { icon: typeof reactTs }> = {

		@@ -2,6 +2,8 @@ export * from './components/Button'

		export * from './components/CypressLogo/CypressLogo'

		export * from './components/FileExplorer/FileExplorer'

wip: cleaning up the specs list and swapping it to be a filetree #15483

wip: cleaning up the specs list and swapping it to be a filetree #15483

Conversation

JessicaSachs commented Mar 15, 2021

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Mar 15, 2021

cypress bot commented Mar 15, 2021

Test summary

Run details

Flakiness

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JessicaSachs Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrKovalenko commented Mar 15, 2021

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmtrKovalenko Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JessicaSachs commented Mar 15, 2021

agg23 commented Mar 15, 2021

dmtrKovalenko commented Mar 15, 2021

dmtrKovalenko Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JessicaSachs commented Mar 15, 2021

lmiller1990 commented Mar 16, 2021 • edited Loading

JessicaSachs commented Mar 16, 2021

JessicaSachs Mar 15, 2021 •

edited

Loading

dmtrKovalenko Mar 15, 2021 •

edited

Loading

dmtrKovalenko Mar 15, 2021 •

edited

Loading

lmiller1990 commented Mar 16, 2021 •

edited

Loading